Report versioning errors in StorageCluster's status #1447

rexagod · 2021-12-21T21:14:50Z

Earlier, only the status of the storage cluster was being updated,
because of which, side effects such as version mismatch came in to play.

rexagod · 2021-12-22T06:43:17Z

https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/red-hat-storage_ocs-operator/1447/pull-ci-red-hat-storage-ocs-operator-main-ocs-operator-bundle-e2e-aws/1473402713831116800#1:build-log.txt%3A157

umangachapagain

I understand why we need to do this, but an operator updating it's own CR is probably not a good idea. We need to re-work on the whole version thing. So, putting this on hold while we explore other options.

/hold

umangachapagain · 2022-02-01T07:17:54Z

It seems late initialization is fine.

Can we patch only the version field from inside version check, instead of updating the entire resource?
If it fails, we'll retry on next reconcile instead of error out.

Changes in this PR can be reverted.
@jarrpa WDYT?

jarrpa · 2022-02-01T15:25:41Z

I would want to see a selective patch rather than a full update. We run the risk of overriding a change that came in right after the current reconciliation event, and we don't want to touch any other spec.

nb-ohad · 2022-02-22T14:17:57Z

@rexagod @jarrpa @umangachapagain

I think Patch is the wrong approach here as it does not guarantee the replacement of the old status with the new status.
We might start to see the strange status values-based as of merge semantics.

I will vote to stay with update and maybe add retries to the status update

rexagod · 2022-02-22T14:32:41Z

/hold

I think I missed Ohad's comment before pushing the latest commit. Nonetheless, I'll put this PR on hold until we reach a common consensus here.

jarrpa

It took some doing but I think I understand the problem with the Patch() strategy now.

In general a Patch() strategy has its place, and it is not here. Specifically this is because the StorageCluster reconcile loop is designed to collect changes to the Status and push them all at once at the end of the reconcile loop (whether or not reconciliation completed successfully is a different story). As such what Ohad is saying is correct, we shouldn't be "merging" new state information, we should be "replacing" the whole thing since we are taking the role as the source of truth on the current state of the system at that point in time.

Now, it is true that we have a bug in how we deal with the Version field in the spec. But also, the way we're using the version field is completely broken, we should never be changing the spec to begin with. I would rather see the version field be dropped entirely and have the "version" information moved to the Status subresource. That will require a bit of design discussion, but shouldn't be too bad.

All that being said, there is still some worthwhile work being done in this PR:

I actually like the idea of having the version string be empty in version.go, so that should be kept.
The versionCheck error condition is also a great addition, so keep that as well.

If you want to change this PR to just those two changes, I'm all right with that. But I would rather have a better conversation on whether we can safely stop using the spec.version field once and for all. We'll have to coordinate with the UI team to know when they'd be able to update the Console code for that, but as it stands it's already broken so what more harm could it do? ¯_(ツ)_/¯

umangachapagain · 2022-03-08T05:25:15Z

* I actually like the idea of having the version string be empty in version.go, so that should be kept.

We use version info to generate a version metric . As long as this doesn't break, I'm fine with it.

ocs-operator/metrics/internal/version/version.go

Lines 1 to 11 in 4f2dfef

    
           package version 
        
           import ( 
        
           	"github.com/red-hat-storage/ocs-operator/version" 
        
           ) 
        
           // GetVersion returns the version of the exporter. 
        
           func GetVersion() string { 
        
           	// version of the Operator 
        
           	return version.Version 
        
           }

rexagod · 2022-03-15T11:34:58Z

I think we'll need to include the version string since ldflags are not in the scope of unit tests, so version.Version will always be "" for them.

https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/red-hat-storage_ocs-operator/1447/pull-ci-red-hat-storage-ocs-operator-main-ocs-operator-ci/1503646574947864576#1:build-log.txt%3A287-292

rexagod · 2022-03-29T07:02:36Z

Bugzilla link: https://bugzilla.redhat.com/show_bug.cgi?id=1855339

openshift-ci · 2022-03-29T07:59:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rexagod, umangachapagain

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [umangachapagain]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2022-03-29T14:46:11Z

New changes are detected. LGTM label has been removed.

rexagod · 2022-03-29T15:07:55Z

/retest

rexagod · 2022-03-29T15:12:07Z

Will /unhold once the e2e tests pass.

rexagod · 2022-05-31T15:27:05Z

/retest
/unhold

Update StorageCluster status on versioning error. Append an error status condition to StorageCluster status field when an error arises during `versionCheck`. Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

openshift-ci · 2022-05-31T17:43:27Z

@rexagod: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ocs-operator-ci	`dfc7a80`	link	true	`/test ocs-operator-ci`
ci/prow/verify-latest-csv	`dfc7a80`	link	true	`/test verify-latest-csv`
ci/prow/red-hat-storage-ocs-ci-e2e-aws	`dfc7a80`	link	false	`/test red-hat-storage-ocs-ci-e2e-aws`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

rexagod · 2022-06-10T08:15:07Z

Follow-up PR: #1706 (spec.version deprecation).

openshift-merge-robot · 2022-09-06T17:32:44Z

@rexagod: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

malayparida2000 · 2022-10-14T07:27:09Z

@rexagod I think this PR is stale now & no longer required. So can we close this?

Nikhil-Ladha · 2023-01-17T07:12:11Z

Looking at the latest bz status, I think we still need this PR.
Also, as the depreciation PR is merged we should be good to test this PR on a nightly/test build, once rebased.

rexagod · 2023-01-22T07:36:53Z

Not sure if we still need this, but since I've moved to a different team recently, I'd appreciate if someone with bandwidth can pick this up.

Closing, TIA.

rexagod force-pushed the 1855339 branch from 44b8f38 to f0903de Compare December 21, 2021 21:18

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2022

umangachapagain requested changes Jan 10, 2022

View reviewed changes

openshift-ci bot assigned umangachapagain Jan 10, 2022

rexagod force-pushed the 1855339 branch 2 times, most recently from ef8fc19 to 55b704d Compare February 22, 2022 13:59

jarrpa requested changes Feb 23, 2022

View reviewed changes

openshift-ci bot assigned jarrpa Feb 23, 2022

rexagod force-pushed the 1855339 branch from ac1994d to 8ac9d92 Compare March 7, 2022 20:33

rexagod changed the title ~~Apply storage cluster changes after reconcilation~~ Report versioning errors in StorageCluster's status Mar 7, 2022

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2022

rexagod force-pushed the 1855339 branch from 8ac9d92 to 2e8c5d0 Compare March 15, 2022 08:17

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2022

rexagod force-pushed the 1855339 branch from 2e8c5d0 to f9313dc Compare March 15, 2022 11:40

umangachapagain approved these changes Mar 29, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2022

rexagod force-pushed the 1855339 branch from f9313dc to a737511 Compare March 29, 2022 14:46

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2022

Report versioning errors in StorageCluster's status

dfc7a80

Update StorageCluster status on versioning error. Append an error status condition to StorageCluster status field when an error arises during `versionCheck`. Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

rexagod force-pushed the 1855339 branch from a737511 to dfc7a80 Compare May 31, 2022 16:17

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2022

rexagod closed this Jan 22, 2023

Nikhil-Ladha mentioned this pull request Jan 23, 2023

Report versioning errors in StorageCluster's Status #1921

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report versioning errors in StorageCluster's status #1447

Report versioning errors in StorageCluster's status #1447

rexagod commented Dec 21, 2021

rexagod commented Dec 22, 2021

umangachapagain left a comment

umangachapagain commented Feb 1, 2022

jarrpa commented Feb 1, 2022

nb-ohad commented Feb 22, 2022 •

edited

rexagod commented Feb 22, 2022

jarrpa left a comment

umangachapagain commented Mar 8, 2022

rexagod commented Mar 15, 2022

rexagod commented Mar 29, 2022

openshift-ci bot commented Mar 29, 2022

openshift-ci bot commented Mar 29, 2022

rexagod commented Mar 29, 2022

rexagod commented Mar 29, 2022

rexagod commented May 31, 2022

openshift-ci bot commented May 31, 2022

rexagod commented Jun 10, 2022

openshift-merge-robot commented Sep 6, 2022

malayparida2000 commented Oct 14, 2022

Nikhil-Ladha commented Jan 17, 2023

rexagod commented Jan 22, 2023 •

edited

Report versioning errors in StorageCluster's status #1447

Report versioning errors in StorageCluster's status #1447

Conversation

rexagod commented Dec 21, 2021

rexagod commented Dec 22, 2021

umangachapagain left a comment

Choose a reason for hiding this comment

umangachapagain commented Feb 1, 2022

jarrpa commented Feb 1, 2022

nb-ohad commented Feb 22, 2022 • edited

rexagod commented Feb 22, 2022

jarrpa left a comment

Choose a reason for hiding this comment

umangachapagain commented Mar 8, 2022

rexagod commented Mar 15, 2022

rexagod commented Mar 29, 2022

openshift-ci bot commented Mar 29, 2022

openshift-ci bot commented Mar 29, 2022

rexagod commented Mar 29, 2022

rexagod commented Mar 29, 2022

rexagod commented May 31, 2022

openshift-ci bot commented May 31, 2022

rexagod commented Jun 10, 2022

openshift-merge-robot commented Sep 6, 2022

malayparida2000 commented Oct 14, 2022

Nikhil-Ladha commented Jan 17, 2023

rexagod commented Jan 22, 2023 • edited

nb-ohad commented Feb 22, 2022 •

edited

rexagod commented Jan 22, 2023 •

edited